-
Notifications
You must be signed in to change notification settings - Fork 514
Temporal: Add test for formatting Temporal objects with era formatter #4634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Awaiting tc39/proposal-temporal#3175 |
ptomato
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
We should have similar tests for formatRange, formatToParts, formatRangeToParts, and the toLocaleString methods of PlainDate, PlainYearMonth, PlainMonthDay, PlainTime, PlainDateTime, Instant, and ZonedDateTime.
test/intl402/DateTimeFormat/prototype/format/temporal-objects-format-with-era.js
Show resolved
Hide resolved
| assert(formatter.format(new Temporal.PlainDate(2025, 11, 4)).startsWith("11"), "formatting a PlainDate should work"); | ||
| assert(formatter.format(new Temporal.PlainYearMonth(2025, 11, "gregory")).startsWith("11"), "formatting a PlainYearMonth should work"); | ||
| assert(formatter.format(new Temporal.PlainMonthDay(11, 4, "gregory")).startsWith("11"), "formatting a PlainMonthDay should work"); | ||
| assert(formatter.format(new Temporal.PlainTime(14, 46)).startsWith("A"), "formatting a PlainTime should work"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit surprised by this expectation... It does match what comes out of new Date().toLocaleTimeString('en', { era: 'narrow' }), but my reading of the spec (GetDateTimeFormat) is that era isn't copied to formatOptions when constructing [[TemporalPlainTimeFormat]].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed tc39/proposal-temporal#3175 (review) accordingly and updated this test.
test/intl402/DateTimeFormat/prototype/format/temporal-objects-format-with-era.js
Outdated
Show resolved
Hide resolved
ptomato
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These look correct to me. One nitpick: all the files in test/built-ins/ should move to test/intl402/ since they are only valid for implementations with Intl.
(Not merging yet because it's marked draft)
@ptomato Fixed.
Ready now! |
ptomato
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
See tc39/proposal-temporal#3049